video: support five streams and improve per-entry access controls - #43
Conversation
An aircraft can carry more than three cameras, and one port carries one stream, so the cap was the limit on how many a single entry could proxy. The three fields that hold per-slot state -- video_ports, video_flags and video_rtmp_path -- all sit in the middle of the record, so none of them could simply grow: every field after them would shift, every record already on disk would be misparsed, and an older binary would read garbage. The append-only contract at the top of keydb.h exists to make that unnecessary, so slots 3 and 4 are carried in new fields appended after reserved[], and accessors join the two halves. A record written before they existed zero-extends into them, which reads as two unused slots, so nothing needs converting and the live database keeps working. video_flags could not be widened for the same reason, and it was already full: three slot bytes plus the entry-wide byte is exactly 32 bits. A fourth slot byte at shift 24 would have landed on the entry options -- which is what happened first time round, and is why there is now a test that sets slots 3 and 4 to 0xFF and checks the audio flag survives. The record grows 344 -> 456 bytes. Also fixes video_port_count() tripping over a short list, which callers that build a KeyEntry by hand were relying on not happening. The README's video section had been pasted in three times: the edit that added it replaced on "## Building", which matches three headings. Only one copy remains.
Bit 3 of each slot's option byte, which was free, so no record growth and no migration -- an existing record reads it clear, which is the current behaviour. It marks a slot whose publisher may be admitted by the entry's MAVLink session even though a publish password is set. Some publishers cannot present one: a camera speaking RTMP from its own firmware has nowhere to put a credential unless its stream-key field tolerates a query, and plain MPEG-TS over UDP never does. Without this an entry faced an all-or-nothing choice between a password and those streams.
admit() gains the slot's session_ok bit. With it set and no credential offered, admission falls through to the MAVLink-session path instead of refusing; without it, nothing changes. The fallback deliberately does not apply to a credential that was offered and is wrong. Downgrading that to address matching would turn a clear rejection into a silent weakening, so a typo cannot succeed on the strength of the source address. Five tests, of which two guard the behaviour being preserved: an unflagged slot still refuses a session-only publisher, and a wrong password is still refused on a flagged one. Verified RED by forcing the bit false -- the three that assert the new path fail, the two guards still pass.
Rendered in the existing per-slot options row and documented in the template beside it, as the other three are. The forms.py tooltip check exempts the per-slot booleans, so the guard that they really are documented where they are rendered is extended to cover it.
Pasting into it with the tooltip showing wedges Chrome's renderer: the tab stops responding to input entirely, and it does not recover. The text it carried moves into the blurb above the form, which already introduced the field, so nothing is lost. The forms.py "every option is documented" guard exempts the field and records why, so it cannot be quietly reinstated.
A tooltip over a password field wedges Chrome's renderer: the tab stops accepting input and does not recover. Reproduced on the login form and removed there; this covers the rest, which share the markup and so presumably share the fault. The help itself is worth keeping -- the publish-password text explains that it replaces the address check, and the new-passphrase one that blank means unchanged -- so the macro renders a password field's description in flow as .field-hint rather than dropping it. The dotted underline that advertises a tooltip goes with it, and aria-describedby is now emitted only when there is something to point at. Two guards: no password input on any page carries a .tip, and the text still reaches the page. Both verified RED.
Previous review (2026-08-25, at head 1174f36)Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting. Reviewed at head ISSUE — a supplied-but-wrong publish password can be read as "absent" and take the address fallback.
Not an access-control bypass — every vector needs the attacker already at the authorized session address on a flag-enabled, non-bidi slot, where credential-free publish is intended — but it means the README's "a typo cannot quietly succeed on the strength of the address" isn't honoured. A uniform fix: represent presence with an explicit flag (independent of the decoded value), require a complete bounded request line before deciding, reject embedded NUL, and use the robust query parser on the publish path too. ISSUE — signed-session fallback can never succeed (fails closed). NOTE — stale comments: Refuted from the cold pass and not issues: the session-name collision check is not still bounded to 3 (every video-slot loop uses Verified clean: append-only ABI (old 344-byte records zero-extend so the new flag defaults off on migration), per-slot/per-entry isolation, port allocation stops correctly at 65535, and the log gate re-validates the viewer each request with traversal closed. Please confirm CI green before merge given this touches an auth path. |
Preserve the distinction between absent and supplied publish credentials across RTSP and RTMP parsing, and export signed MAVLink authentication state for bidi session fallback. Update stale five-slot schema documentation and add regression coverage.
Previous review (2026-08-25, at head 42209cf)Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting. Full report: https://uav.tridgell.net/DevCallReviews/2026_08_26/devcall_pr_reviews.html#prSupportProxy_43 Reviewed at head Re-reviewed at the same head as my 2026-08-25 comment; the auth/ABI work verified clean again (credential-presence bits end-to-end, 456-byte split-record ABI offsets, per-slot session_ok, RTSP request guarding), but the must-fix from that round is still unaddressed, so the verdict stands. Must fix:
Should fix:
Notes (non-blocking):
Previous round triage: SESSION_EXTS BUG — still open (impact narrowed on re-analysis: |
Reject duplicate RTMP connect properties, preserve the first credential across RTMP setup commands, and continue validating credentials on every RTSP request. Bound RTSP framing, reject ambiguous content lengths, and cover the reported downgrade cases with integration tests.
A per-slot flag that admits a publisher offering no credential with no MAVLink-session check and no password. For a camera on its own link whose telemetry does not pass through this proxy and which cannot carry a password (plain MPEG-TS/UDP). Off by default; a password that is offered and wrong is still refused.
A publisher sending rtph264pay/rtph265pay ! udpsink (or ffmpeg -f rtp) to a video port was counted as bad datagrams: the UDP path only knew MPEG-TS. Now an RTP datagram from the latched publisher is recognised, the codec is read off the first unambiguous NAL header, and the datagrams are forwarded to the same ffmpeg backend RTSP uses, fed an SDP on stdin naming the codec and a loopback port. Its MPEG-TS output goes through the normal ingest, so viewers and the recorder are unchanged. Parameter sets must be in-band, as there is no SDP from the sender. The backend is told to bind 127.0.0.1 only -- the sdp demuxer's default is 0.0.0.0, which would expose the port past admission -- and start() waits for that bind so the first datagrams are not lost. connections.tdb rows report such a publisher as RTP over UDP.
basename_free() listed .v1.ts to .v3.ts, so a name taken only by a slot 4 or 5 recording was handed out again and two sessions shared a basename. Derive the list from KEY_MAX_VIDEO_PORTS instead.
On a public-logs entry anyone can open play.mp4, and each stream holds a gunicorn thread and an ffmpeg for as long as the client reads. With four threads shipped, a few slow anonymous readers took the whole web UI. Anonymous readers now get two concurrent remuxes and a 503 with Retry-After beyond that; logged-in readers are unaffected.
tridge
added
AIReview
Previous review (2026-09-07, at head `8e398f3b36`)Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_07_1655/devcall_pr_reviews.html#prSupportProxy-43 Reviewed at head Re-reviewed on request; my earlier comment above is superseded. All four findings from the last round (head Previous round
Must fixBUG — two anonymous Since start_webadmin.sh:44 runs BUG — a dead RTP backend is never cleaned up, leaving a permanent 100%-CPU epoll spin. This is deterministic, not a race: Should fix
Notes (non-blocking)
Verified cleanRecorded so the clearances are on the record too: |
A GET route also answers HEAD, and Werkzeug never starts the body generator for one, so cleanup that lived only in the generator's finally never ran: each anonymous HEAD leaked an ffmpeg and one of the two anonymous permits, and two of them disabled anonymous playback until restart. Cleanup is now idempotent and also hooked on call_on_close, with the permit released in its own finally.
close_rtsp() returned early once reap() had marked the backend gone, because an RTP publisher has neither a client fd nor an RTMP session: the media fd stayed in epoll and spun on EPOLLHUP. The guard now also keys on the media fd. RtspBackend::start() for RTP reaps during the readiness wait and fails on exit or timeout, as the RTSP path does, so the retry backoff is armed and a dead backend is never registered. Also: RTP is only considered before any TS has been scanned, so a stray RTP-shaped datagram cannot abandon a live MPEG-TS stream, and an oversize datagram is dropped (MSG_TRUNC) rather than forwarded truncated.
It is judged against the entry's publish password, so with none set and open_publish on, a supplied one is accepted unchecked.
|
Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_07_1910/devcall_pr_reviews.html#prSupportProxy-43 Reviewed at head Re-reviewed on request; my earlier comment above is superseded. All four findings from the last round (head Everything load-bearing here was checked by mutation: each fix reverted, rebuilt, and the behaviour re-measured. That is also how I found the one weak spot (a new regression test that still passes without its fix). Previous round — all resolved
Still open from before, unchanged and still non-blocking: the H.264 fallback cannot self-correct (video.cpp#L611). Re-raising only so it is not silently dropped. Correction to my previous commentI described finding 2's symptom as "a permanent 100%-CPU epoll spin" with an fd leak that "outlives the publisher". Measured against the pre-fix build, that is not what happens, and I should not have stated it that confidently. The video child used only 0.05 s of CPU across the whole scenario, and the slot does recover — the hangup on the orphaned media pipe reaches New — should fixA partial RTSP request line spins the video child at ~75% of a core for 2 s, pre-auth. The guard returns Scope cuts both ways, so both halves are worth stating. The spin is pre-existing: a payload matching no detector at all ( The That is the primary use of Notes (non-blocking)
Verified cleanTree builds clean under the project's full |
Detect leaves an incomplete RTSP or HTTP request line in the socket on purpose, since the credential may be in the next segment. With level-triggered EPOLLIN those bytes re-fire on every epoll_wait until the 2 s detect timeout, which measured as most of a core. The viewer now says when it is holding bytes and the child arms EPOLLET for that fd until it is classified, so it only wakes when more arrive. Also: the UDP receive buffer is the full datagram size, so MSG_TRUNC drops are unreachable in practice, and the RTP lifecycle test checks the child's fd count does not grow across backend restarts.
require_log_read redirected to /login?next=..., but the form posted to a bare /login and login() reads next from the query string, so a Login Required share link always landed on the index.
The session fixture returns on "listening", which is logged before "video child N ready"; on a loaded CI runner the pid was not in the log yet and the CPU-spin test failed on a missing match.
1 participant
Summary
Compatibility
Existing key records remain valid: slots four and five use fields appended after the established record layout, and the new access flags default existing entries to their previous private behavior.
Testing